Avoid intermediate DAGCircuit construction in 2q synthesis #12179
Conversation
mtreinish
added
on hold
mtreinish
requested review from
alexanderivrii,
ShellyGarion and
a team
as code owners
|
One or more of the the following people are requested to review this:
|
|
I ran some quick asv benchmarks and it's showing a nice modest performance improvement over #12179 |
|
The other question for me is whether we want to have logic to default to the old mode. I did a quick benchmark comparing the runtime of a 1000 gate 2q circuit (I probably should do a deeper example as this isn't super realistic) where it was either a cx gate or a random unitary and did a sweep over the percentages. It looks like there is an crossover point at slightly less than 2%:
It wouldn't be very hard to compute the percentage of unitary gates in the circuit and use the old code (without this PR in it's current form) if it's |
Pull Request Test Coverage Report for Build 8877774475Details
💛 - Coveralls |
There was a problem hiding this comment.
Mostly LGTM, I just left one small comment in the code, and as mentioned in #12195, I like the idea of using the old code to gain a bit of speed on the cases with fewer unitaries. I think it's a use case that might come up when someone builds a benchmark, but if there's no time before the release deadline I also think it's fine to do it as a follow-up.
| getattr(circ, name)(*params, *qubits) | ||
| except AttributeError as exc: | ||
| if use_dag: | ||
| from qiskit.dagcircuit.dagcircuit import DAGCircuit |
There was a problem hiding this comment.
This was added to avoid the cyclic import complaints, right? In #12203 complaints I had too many of those and I opted for moving the synthesis imports in qiskit.circuit.library to import at runtime.
There was a problem hiding this comment.
I think moving the synthesis imports to runtime make the most sense. I wrote this for #12109 so I don't really remember the exact cause of the import cycle. But I wouldn't have used an import runtime unless I needed to avoid a cycle.
| elif name == "u3": | ||
| gate = U3Gate(*params) | ||
| circ.append(gate, qubits) | ||
| elif name == "u2": | ||
| gate = U2Gate(*params) | ||
| circ.append(gate, qubits) | ||
| elif name == "u1": | ||
| gate = U1Gate(*params) | ||
| circ.append(gate, qubits) |
There was a problem hiding this comment.
Is there any reason why you didn't use GATE_NAME_MAP here too?
There was a problem hiding this comment.
This was actually left over from #12109 . I've rebased the PR now and this isn't here anymore. TBH, I'm not sure why I didn't use the dict here too. I guess my thinking at the time was for the QuantumCircuit path we can use the gate methods on the QuantumCircuit class except for these 4 cases. But, also I was also likely trying to minimize the diff in #12109 and not really touch the circuit path. I can go back and follow up and deduplicate this in a separate PR if you'd like though.
This commit builds on Qiskit#12109 which added a dag output to the two qubit decomposers that are then used by unitary synthesis to add a mode of operation in unitary synthesis that avoids intermediate dag creation. To do this efficiently this requires changing the UnitarySynthesis pass to rebuild the DAG instead of doing a node substitution.
mtreinish
force-pushed
the
no-dag-2q-synth
branch
from
c3e4458
to
59d1532
Compare
|
Yep, my bad it was leftover from when this was waiting for #12109 to merge. |
) This commit builds on Qiskit#12109 which added a dag output to the two qubit decomposers that are then used by unitary synthesis to add a mode of operation in unitary synthesis that avoids intermediate dag creation. To do this efficiently this requires changing the UnitarySynthesis pass to rebuild the DAG instead of doing a node substitution.
Summary
This commit builds on #12109 which added a dag output to the two qubit
decomposers that are then used by unitary synthesis to add a mode of
operation in unitary synthesis that avoids intermediate dag creation.
To do this efficiently this requires changing the UnitarySynthesis pass
to rebuild the DAG instead of doing a node substitution.
Details and comments
This depends-on #12109 and will need to be rebased after that merges